-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reworking Sorting Module for Consistency #61
Conversation
I hope one can squash commits in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great job, thank you for the thorough review & contribution ❤️
I had to revert commit c9a3ea6 as it did not successfully sort descending arrays.
Oh, good catch! I missed this 🚀
just use a Sorter Trait everywhere, and then provide two methods: sort and sort_inplace
Could you elaborate on why you didn't just use one Sorter
trait? I was thinking something like:
pub trait Sorter<T: Ord + Copy> {
fn sort_inplace(arr: &mut [T]);
fn sort(arr: &[T]) -> Vec<T> {
let mut result = arr.to_vec();
Self::sort_inplace(&mut result);
result
}
}
I think I would rather have the trait implementation after the sorting function itself. That is, if you were to read top-to-bottom, you would find the algorithm first, and then the abstracting traits. This appeals better to someone new to Rust/someone that just wants to read the algorithm itself. What do you think?
I hope one can squash commits in the PR.
Yeah, no worries, I always squash before merging and give it an appropriate name 👍🏻
I think in the beginning I wanted to avoid having to require the trait Let me know if I should change it to your proposal. I do see that yours is a lot easier to read.
Yeah, that sounds like a good idea. Will do! |
Co-authored-by: Alexander González <[email protected]>
Ah, what the heck. I am basically using Copy already everywhere. I'll use your proposal! |
…to top of the file for readability
Alright, applied the suggestions. Requesting review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Excellent PR tbh, I see you did quite a few small things in addition like removing some casts and removing some unnecessary trait bounds. Thank you!
See Issue: #37
I had to revert commit c9a3ea6 as it did not successfully sort descending arrays.
I based my rework on the proposal in #37 (comment)
Note that inplace here does not refer to whether the actual sorting algorithm is inplace, i.e. requires no extra space (https://www.geeksforgeeks.org/sorting-terminology/), but rather do denote whether the output is stored in the same memory location of the input or not.
Let me know whether these changes follow good practice and are practical.